Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: re-factor the CodePipeline Action bind method #2085

Merged
merged 1 commit into from
Mar 29, 2019
Merged

feat: re-factor the CodePipeline Action bind method #2085

merged 1 commit into from
Mar 29, 2019

Conversation

skinny85
Copy link
Contributor

The role it gets passed is no longer directly tied to the Pipeline
(although it still might be the Pipeline Role, behind the scenes).
That's needed for some more advanced use-cases, like cross-account Pipelines.

Changed to also have the method accept an interface,
to facilitate future evolution.

Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@skinny85 skinny85 requested a review from eladb March 23, 2019 00:09
@skinny85 skinny85 requested review from RomainMuller and a team as code owners March 23, 2019 00:09
eladb
eladb reviewed Mar 24, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand if this makes it easier or harder to implement custom actions (see #2073), which I believe we should address. Maybe we should think about those two changes together?

@skinny85
Copy link
Contributor Author

I am not sure I understand if this makes it easier or harder to implement custom actions (see #2073), which I believe we should address. Maybe we should think about those two changes together?

It doesn't affect the custom actions at all - it's completely unrelated. I'd rather not combine these two changes.

@eladb
Copy link
Contributor

eladb commented Mar 25, 2019

It doesn't affect the custom actions at all - it's completely unrelated. I'd rather not combine these two changes.

I am okay not combining, but I'd like to discuss the full API before we proceed here.

@skinny85
Copy link
Contributor Author

I am okay not combining, but I'd like to discuss the full API before we proceed here.

Sure. What are your thoughts?

@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 7c48e82 to 7416e9f Compare March 26, 2019 20:26
@skinny85
Copy link
Contributor Author

Rebased on top of newest master.

@skinny85 skinny85 mentioned this pull request Mar 26, 2019
Merged
4 tasks
eladb
eladb approved these changes Mar 28, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, conditional of converting (basically just renaming) IBindInfo to BindInfo. Perhaps rename to just ActionBind.

packages/@aws-cdk/aws-codepipeline-api/lib/action.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts Show resolved Hide resolved
@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 7416e9f to 4e9d39c Compare March 28, 2019 22:57
@skinny85
feat(codepipeline): re-factor the CodePipeline Action bind method t…
5f138fd 
…o take a Role separately from the Pipeline.
@skinny85 skinny85 force-pushed the feature/pipeline-actions-bind-api-refactor branch from 4e9d39c to 5f138fd Compare March 28, 2019 23:08
@skinny85
Copy link
Contributor Author

skinny85 commented Mar 28, 2019 •
edited
Loading

Rebased, and re-named IBindInfo to ActionBind.

@skinny85 skinny85 merged commit ffe0046 into aws:master Mar 29, 2019
@skinny85 skinny85 deleted the feature/pipeline-actions-bind-api-refactor branch March 29, 2019 17:09
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
This was referenced Dec 12, 2019
Closed
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

@RomainMuller RomainMuller Awaiting requested review from RomainMuller

Assignees

@skinny85 skinny85

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@skinny85 @eladb @RomainMuller @NGL321